Skip to content

fix(core): FileLoader support .mjs as first-class#6023

Merged
fengmk2 merged 3 commits into
eggjs:nextfrom
elrrrrrrr:fix/file-loader-support-mjs
Jul 7, 2026
Merged

fix(core): FileLoader support .mjs as first-class#6023
fengmk2 merged 3 commits into
eggjs:nextfrom
elrrrrrrr:fix/file-loader-support-mjs

Conversation

@elrrrrrrr

@elrrrrrrr elrrrrrrr commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

What

Add .mjs to FileLoader's default match glob so .mjs files are scanned the same as .js/.ts.

- return isSupportTypeScript() ? ['**/*.(js|ts)', '!**/*.d.ts'] : ['**/*.js'];
+ return isSupportTypeScript() ? ['**/*.(js|ts|mjs)', '!**/*.d.ts'] : ['**/*.{js,mjs}'];

Why

The default match only includes .js/.ts, so .mjs files are silently skipped. egg4 plugins built as ESM (e.g. tsdown/tsc emitting .mjs) put their app/middleware, app/extend, config etc. in .mjs. Those never get loaded → app.middlewares is missing the plugin's middleware, and a lookup like app.middlewares[name] throws Middleware xxx not found.

Concretely, an ESM plugin shipping app/middleware/xxx.mjs and pushing xxx into config.tr.middleware (or coreMiddleware) currently fails to boot because the middleware file is never scanned.

In type: module packages .js is already ESM, but many build toolchains emit .mjs explicitly. Treating .mjs as first-class avoids forcing every ESM plugin to re-emit as .js.

Status

Draft — opening for discussion / direction. Happy to add tests + cover the .mjs load path if this direction looks right.

Summary by CodeRabbit

  • New Features

    • Expanded file loading support to include .mjs and .cjs files in addition to .js, with TypeScript projects also recognizing .ts sources.
    • When both source and compiled versions exist, the TypeScript file now takes precedence.
  • Bug Fixes

    • Prevented compiled .js, .mjs, and .cjs files from being loaded alongside matching TypeScript files, avoiding duplicate attachments.

The default FileLoader match glob only includes `.js`/`.ts` (and `.js` when
TypeScript is off), so `.mjs` files are silently skipped. egg4 plugins built
as ESM (e.g. via tsdown/tsc emitting `.mjs`) put their `app/middleware`,
`app/extend`, `config` etc. in `.mjs`, which then never get loaded — e.g.
`app.middlewares` ends up missing plugin middleware, and any lookup like
`app.middlewares[name]` throws "Middleware xxx not found".

Add `.mjs` to the default match so ESM plugins shipping `.mjs` are scanned
the same as `.js`. In `type: module` packages `.js` is already ESM, but many
build toolchains emit `.mjs` explicitly; treating `.mjs` as first-class avoids
requiring every ESM plugin to re-emit as `.js`.
@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f3b4c420-ea80-4171-af52-517b6c8d92a1

📥 Commits

Reviewing files that changed from the base of the PR and between d55efa3 and 2452d45.

📒 Files selected for processing (8)
  • packages/core/src/loader/file_loader.ts
  • packages/core/test/egg-ts.test.ts
  • packages/core/test/fixtures/egg-ts-js/app/service/dual.mjs
  • packages/core/test/fixtures/egg-ts-js/app/service/dual.ts
  • packages/core/test/fixtures/egg-ts-js/app/service/dualc.cjs
  • packages/core/test/fixtures/egg-ts-js/app/service/dualc.ts
  • packages/core/test/fixtures/egg-ts-js/app/service/pureCjs.cjs
  • packages/core/test/fixtures/egg-ts-js/app/service/pureMjs.mjs

📝 Walkthrough

Walkthrough

The change extends FileLoader to match and load .mjs/.cjs files alongside .js/.ts, and expands the ignore logic to skip compiled .js/.mjs/.cjs files when a corresponding .ts source exists. New fixture files and a test case validate discovery and precedence behavior.

Changes

mjs/cjs loading support

Layer / File(s) Summary
File loader glob matching and ignore logic
packages/core/src/loader/file_loader.ts
Default glob patterns now include mjs/cjs in both TypeScript-enabled and disabled modes; ignore logic extended to skip .js, .mjs, and .cjs compiled files when a matching .ts sibling exists.
Fixtures and test verifying precedence
packages/core/test/egg-ts.test.ts, packages/core/test/fixtures/egg-ts-js/app/service/{pureMjs.mjs,pureCjs.cjs,dual.ts,dual.mjs,dualc.ts,dualc.cjs}
New fixture services define loadedFrom markers for mjs/cjs/ts sources; a new test verifies standalone .mjs/.cjs loading and confirms .ts sources take precedence over compiled siblings.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

Not applicable — the changes are focused loader logic and fixture/test additions without multi-component orchestration warranting a sequence diagram.

Suggested reviewers: fengmk2, killagu

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title correctly captures the main change: FileLoader now treats .mjs as a first-class match.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the default file loader match pattern to include .mjs files alongside .js and .ts files. The reviewer identified a potential issue where adding .mjs support could lead to duplicate loading of both .ts and compiled .mjs files, as the existing deduplication logic only checks for .js files. It is recommended to update the deduplication check to also handle .mjs files to prevent application crashes.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread packages/core/src/loader/file_loader.ts Outdated
@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.97%. Comparing base (d55efa3) to head (2452d45).

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #6023   +/-   ##
=======================================
  Coverage   81.96%   81.97%           
=======================================
  Files         678      678           
  Lines       20800    20800           
  Branches     4147     4147           
=======================================
+ Hits        17048    17050    +2     
+ Misses       3235     3233    -2     
  Partials      517      517           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…s/.cjs

- Add `.cjs` to the default FileLoader match alongside `.mjs`.
- Extend the source-wins de-dup (previously `.js`-only) to `.mjs`/`.cjs`:
  when a `.ts` source and its compiled `.js`/`.mjs`/`.cjs` sibling coexist in
  the same directory (e.g. tsc/tsdown output kept beside the source during
  development), the `.ts` source wins and the compiled sibling is ignored —
  otherwise both are attached to the same property and `can't overwrite
  property` is thrown. (per @gemini-code-assist review)

Priority when several extensions coexist for one basename:
  `.ts` (source) > compiled `.js`/`.mjs`/`.cjs`.

Adds fixtures + a test covering standalone .mjs/.cjs loading and the
.ts-over-.mjs / .ts-over-.cjs de-dup.
@elrrrrrrr

Copy link
Copy Markdown
Contributor Author

Thanks @gemini-code-assist — good catch, fixed in 4a2c566.

Extended the "source wins" de-dup (previously .js-only) to .mjs/.cjs, so a compiled sibling is ignored while the .ts source exists:

if (filepath.endsWith('.js') || filepath.endsWith('.mjs') || filepath.endsWith('.cjs')) {
  const filepathTs = filepath.replace(/\.(?:js|mjs|cjs)$/, '.ts');
  if (filepaths.includes(filepathTs)) continue;
}

Priority when several extensions coexist for one basename: .ts (source) > compiled .js/.mjs/.cjs. Coexistence among the compiled formats themselves (e.g. foo.js + foo.mjs with no .ts) is a build misconfiguration and still surfaces the existing can't overwrite property error deliberately.

Also added .cjs to the default match (alongside .mjs) and a test in egg-ts.test.ts covering standalone .mjs/.cjs loading plus the .ts-over-.mjs / .ts-over-.cjs de-dup.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Thanks for the update, @elrrrrrrr. The logic for handling the source file priority and the addition of .cjs support looks solid and addresses the potential for duplicate loading when compiled files coexist with their .ts sources. The added tests in egg-ts.test.ts provide good coverage for these scenarios. This looks like a complete and well-thought-out solution.

@elrrrrrrr elrrrrrrr requested review from fengmk2 and killagu July 7, 2026 08:55
@elrrrrrrr elrrrrrrr marked this pull request as ready for review July 7, 2026 08:55
Copilot AI review requested due to automatic review settings July 7, 2026 08:55

@killagu killagu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Egg core’s FileLoader default glob matching so that .mjs files (and, in the current implementation, .cjs as well) are treated as first-class load targets alongside .js/.ts, and adds regression fixtures/tests to ensure .ts sources take precedence over adjacent compiled outputs.

Changes:

  • Expand FileLoader default match patterns to include .mjs/.cjs, and ignore compiled siblings when a .ts source exists.
  • Add fixture services for standalone .mjs/.cjs and “dual” .ts + compiled output coexistence.
  • Add a test asserting .mjs/.cjs loading and .ts-over-compiled precedence.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/core/src/loader/file_loader.ts Extend default glob match to include .mjs/.cjs and skip compiled siblings when .ts exists.
packages/core/test/egg-ts.test.ts Add a regression test around .mjs/.cjs loading and precedence rules.
packages/core/test/fixtures/egg-ts-js/app/service/pureMjs.mjs New standalone .mjs service fixture.
packages/core/test/fixtures/egg-ts-js/app/service/pureCjs.cjs New standalone .cjs service fixture.
packages/core/test/fixtures/egg-ts-js/app/service/dual.ts New .ts source fixture intended to win over compiled sibling.
packages/core/test/fixtures/egg-ts-js/app/service/dual.mjs New compiled .mjs sibling fixture intended to be ignored when .ts exists.
packages/core/test/fixtures/egg-ts-js/app/service/dualc.ts New .ts source fixture intended to win over compiled sibling.
packages/core/test/fixtures/egg-ts-js/app/service/dualc.cjs New compiled .cjs sibling fixture intended to be ignored when .ts exists.

Comment on lines 66 to 68
function getDefaultFileLoaderMatch(): string[] {
return isSupportTypeScript() ? ['**/*.(js|ts)', '!**/*.d.ts'] : ['**/*.js'];
return isSupportTypeScript() ? ['**/*.(js|ts|mjs|cjs)', '!**/*.d.ts'] : ['**/*.{js,mjs,cjs}'];
}
Comment on lines +130 to +150
it('should load mjs/cjs and prefer the ts source over its compiled mjs/cjs', async () => {
mm(process.env, 'EGG_TYPESCRIPT', 'true');
app = createApp('egg-ts-js');

// loadService must not throw "can't overwrite property" even though
// `dual.ts`+`dual.mjs` and `dualc.ts`+`dualc.cjs` coexist in the directory.
await app.loader.loadService();

// standalone `.mjs` / `.cjs` are scanned and loaded as first-class files
assert(app.serviceClasses.pureMjs);
assert.equal(app.serviceClasses.pureMjs.loadedFrom, 'mjs');
assert(app.serviceClasses.pureCjs);
assert.equal(app.serviceClasses.pureCjs.loadedFrom, 'cjs');

// priority: when a `.ts` source and its compiled `.mjs`/`.cjs` coexist,
// the `.ts` source wins and the compiled sibling is ignored.
assert(app.serviceClasses.dual);
assert.equal(app.serviceClasses.dual.loadedFrom, 'ts');
assert(app.serviceClasses.dualc);
assert.equal(app.serviceClasses.dualc.loadedFrom, 'ts');
});
@fengmk2

fengmk2 commented Jul 7, 2026

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 2452d45428

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@fengmk2 fengmk2 merged commit a71ef2b into eggjs:next Jul 7, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants